Add a source capture host example and sync the documentation - #117
Add a source capture host example and sync the documentation#117chrisuthe wants to merge 7 commits into
Conversation
8f828e6 to
09cc38b
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Critical authorization and lifetime defects, plus callback-safety and failure-handling issues, must be resolved before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a host source-capture example using PortAudio, with PCM/Opus streaming, optional mDNS, and synchronized documentation.
Changes:
- Introduces the
source_clientexample and build configuration. - Documents source-role APIs, internals, usage, and build gating.
- Updates project architecture and feature summaries.
File summaries
| File | Review |
|---|---|
README.md |
Lists source support and the new example. No findings. |
examples/source_client/README.md |
Documents building and running the example. No findings. |
examples/source_client/main.cpp |
Critical (2 votes), L433: Enforce source authorization before exposing privacy-sensitive input. Critical (1 vote), L423: Construct provider/listener before the client to satisfy lifetime contracts. Moderate (1 vote), L255: Avoid mutexes and logging in the real-time audio callback. Moderate (1 vote), L406: Exit the main loop when Pa_StartStream() fails.Moderate (1 vote), L462: Use a steady-clock deadline and one interval constant. Nit (1 vote), L257: Correct the inaccurate no-logging claim. Nit (1 vote), L76: Extract the duplicated mDNS advertiser into shared example code. |
examples/source_client/CMakeLists.txt |
Configures PortAudio and mDNS dependencies. No findings. |
docs/internals.md |
Documents source pipeline internals. No findings. |
docs/integration-guide.md |
Nit (1 vote), L189: Document pairing/trust requirements and consumer-facing pairing setup. |
docs/conventions.md |
Clarifies build gating for single-role examples. No findings. |
CMakeLists.txt |
Conditionally includes the source example. No findings. |
CLAUDE.md |
Updates architecture documentation. No findings. |
Review details
Suppressed comments (3)
docs/integration-guide.md:189
- This presents the server start command as the only streaming gate, but the linked source@v1 requirements also require a paired (
usertrust) connection and mandate refusal at trustnone. Add that requirement and the consumer-facing pairing setup here; the current implementation has no trust check, so this documentation currently describes a non-compliant and unsafe contract.
Streaming is gated by the server: the client never streams unsolicited, the default after connect is stopped, and permission does not survive reconnection. When the server commands start, the role opens the outbound stream and fires `on_streaming_started()`; from that point on, feed captured audio to `write_audio()`:
examples/source_client/main.cpp:259
- The “no logging on the audio callback” claim is false: this callback calls
SourceRole::write_audio(), whose ring-full path logs atsrc/source_task.cpp:207-218. Update the comment so users are not told this path is log-free.
// Counted here and reported from the main loop: no logging on the audio
// callback. A rejected write means the capture ring is full, or the stream
// closed while this callback was in flight.
examples/source_client/main.cpp:77
- This adds a third copy of the existing
MdnsAdvertiserimplementation (examples/basic_client/main.cpp:67-118andexamples/tui_client/main.cpp:79-110). That conflicts with the reuse-before-invention rule indocs/conventions.md:106-109and lets registration/error handling drift independently. Extract the advertiser intoexamples/commonand reuse it from all host examples.
// Manages mDNS service advertisement via dns_sd.h
class MdnsAdvertiser {
- Files reviewed: 9/9 changed files
- Comments generated: 5
- Review effort level: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| CaptureSourceListener source_listener(capture); | ||
| HostNetworkProvider network_provider; |
There was a problem hiding this comment.
Fixed in 842d16a: the listener and network provider are declared (with the capture they wrap) before the client, so destruction order honors the lifetime contracts.
| fprintf(stderr, "Starting Sendspin source client on port %u (%s)...\n", server_port, | ||
| use_opus ? "opus" : "pcm"); | ||
|
|
||
| if (!client.start_server()) { |
There was a problem hiding this comment.
Core enforcement cannot exist on this protocol revision (no pairing transport, no trust state — the stack's documented sequencing caveat; see the reply on #114 for the full rationale). What this round adds in 842d16a: a startup warning in the example that any handshaken server can start capture, and an authorization note in the integration guide saying the same and recommending trusted networks for microphone inputs until the encryption work lands.
| if (!self->source_->write_audio(static_cast<const uint8_t*>(input), | ||
| frame_count * self->bytes_per_frame_, capture_us)) { |
There was a problem hiding this comment.
Split resolution: the header doc now states the per-platform truth (0b51d56) — the ring is lock-free on ESP, the production target, while the host implementation takes a short mutex-guarded critical section, so hard-real-time host callers should stage through their own lock-free handoff. Rebuilding the host platform ring lock-free for an example-grade capture path isn't warranted; the example's callback comment no longer claims the path is log-free (the library warns once per overflow episode from that thread, 842d16a).
| void on_streaming_started() override { | ||
| fprintf(stderr, ">>> Streaming started\n"); | ||
| if (!capture.start()) { | ||
| fprintf(stderr, ">>> Failed to start capture\n"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Fixed in 842d16a: a failed Pa_StartStream now exits the main loop so shutdown closes the outbound stream instead of leaving the server waiting on silence.
| int tick = 0; | ||
| while (running.load()) { | ||
| client.loop(); | ||
| // Surface capture drops off the audio callback (which only counts them) | ||
| if (++tick % 500 == 0) { | ||
| uint32_t dropped = capture.take_dropped_writes(); | ||
| if (dropped > 0) { | ||
| fprintf(stderr, ">>> Dropped %u capture writes in the last 5 s\n", dropped); | ||
| } | ||
| } |
There was a problem hiding this comment.
Fixed in 842d16a: the drop report runs on a steady-clock deadline with one named interval constant.
The example declares its listener and network provider before the client (their raw pointers must outlive it), exits the main loop when capture fails to start so shutdown closes the open stream, reports capture drops on a steady-clock interval instead of counted loop ticks, warns at startup that any handshaken server can start capture on this protocol revision, and corrects the audio-callback logging comment. The integration guide gains the matching authorization note and the 20 ms default with 5 ms Opus support; internals documents the binary send worker, its slot, and its reclamation path.
Part 6/6 of the source@v1 stack (tracker: #95).
What it adds:
examples/source_client(captures the default PortAudio input and streams it, with an Opus switch) and the documentation sync — integration guide, internals, CLAUDE.md, README.How it's used: build with examples on and run it against a Sendspin server; the README in the example folder has the details.
Note for maintainers: this also adds one clarifying sentence to
docs/conventions.md— an example that exists solely to demonstrate one role may gate its whole CMake target on that role's option (an#ifdefleaving a do-nothing binary is dead code, not a guard). Flagged here for explicit sign-off; drop it if you prefer the rule implicit.